Rad. Makes a lot of sense.

I like the approach. It seems pretty clean. I'd like to see some more automated testing, if possible, and it would also be good to verify that it actually works (like, with a real app and a real proxy). Just a one time manual test is fine, I just want to validate the end-to-end.

I have some sample code (in both Android and plain Java) to exercise this SDK, but unfortunately it's not up-to-date on GitHub. If it would be helpful to get those in your hands, let me know and we can sync up on the best way to do that.

I'm planning on adding an automated test as part of the core/.../UrlConnectionHttpHandlerTest.java file that verifies that the proxy gets used if it is set. Do you think it's worth going beyond that and fully mocking a proxy in the test suite?

I did verify it by hand with a real app and real proxy. It was a bit finicky because it required that the proxy support tunneling HTTPS traffic (which, I came to find out, many don't), however if they don't want that behavior, they can simply use the setBaseUrl functionality and point it at the proxy directly.

Also, should I add documentation to the readme? This seems like pretty advanced functionality, so I wasn't sure if the readme was the right place.

I think you will find that for a lot of larger corporate users or companies that have tight security, proxies are quite common. So I think it would be worth a mention on the readme..even if it's just one line with a link to a specific wiki page on how to use it.

Hmm, I think that just verifying that it gets used is probably good enough for now. If we run into issues in the wild then we can flesh out the test behavior.

Glad to hear that you manually verified the behavior. That always makes me a lot more comfortable pushing something out :)

In terms of documentation, I think @ryber is right that it might be worth a mention. As I may have said before, I have some significant doc improvements in the works so I can try to handle this better whenever I get a day or two to push through and finish/deploy those. For now a simple mention in the main README should be fine. Thanks!

I've been working on adding a test to ensure the Proxy gets used and I've come to a bit of a dead end. The issue is that the 5 lines in question end up calling methods of the URL object, which is a final class, so it can't be mocked and it isn't an implementation of an interface, so it can't simply be replaced with dependency injection.

Any other ideas on how to handle this or should we just put it aside and reconsider it if issues crop up in the future?

Ah, yuck. Seems like a lot of hassle for low return, I'd have no problem punting on it.

Yeah, I feel like I've run into that problem as well. I'm OK punting on it, we'll just have to keep our ears open for any issues that people have.

Other than that, is this PR ready to go?

Yep, good to go.

👍

Thanks Simon. I'll need to do a bit of work to push a 2.0.3 release up to maven central. I'll try to get to that ASAP. (If you have interest in improving automation around releases, by the way, we can definitely talk about that as a project at some point ;) )

FYI, 2.0.3 has been released. I'm going to work on some sample app stuff tomorrow as part of a documentation push, so I'll use that to smoke-screen the release as well.